Validate Treelite model inputs during import - #104

cpp/include/nvforest/detail/node.hpp (1)

217-221: ⚡ Quick win

Update the node docs to describe the new validation behavior.

The public comment above node still says construction does “NO error checking” and will “silently” build an incorrect node, but construct_metadata() now rejects out-of-range feature values and throws model_import_error. Please sync the Doxygen with the actual constructor behavior, including the feature bound and thrown exception.

As per coding guidelines, "For public header files (C++ API): Verify parameter descriptions match actual types/behavior" and "Flag API changes that may need corresponding documentation updates".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cpp/include/nvforest/detail/node.hpp` around lines 217 - 221, Update the
Doxygen comment above the node type to reflect that construct_metadata() now
validates the feature value and will throw model_import_error for out-of-range
values: document the feature bound (must be <= FEATURE_MASK), state that the
constructor/construct_metadata() performs runtime checking rather than “NO error
checking” and will raise model_import_error on violation, and ensure parameter
descriptions and exception tags match the actual types/behavior (mention
construct_metadata(), FEATURE_MASK, and model_import_error).

cpp/include/nvforest/exceptions.hpp (1)

15-17: ⚡ Quick win

Broaden the public docs for model_import_error.

After folding validation failures into model_import_error, this type no longer represents only import-time failures. node::construct_metadata() now throws it for generic model validation as well, so the current /** Exception indicating model import failed */ description is too narrow for downstream users. Please update the public API docs/comments to reflect the broader contract.

As per coding guidelines, "For public header files (C++ API): Check if new public functions/classes have documentation comments" and "Flag API changes that may need corresponding documentation updates".

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cpp/include/nvforest/exceptions.hpp` around lines 15 - 17, Update the
documentation comment for the exception type model_import_error in
exceptions.hpp to reflect that it is used for both import-time failures and
broader model validation errors (e.g., those thrown by
node::construct_metadata()), not just import-only failures; edit the public
header comment above the model_import_error class/constructor to describe this
broader contract and ensure it matches coding guidelines for public C++ API
docs.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

cpp/include/nvforest/detail/decision_forest_builder.hpp (1)

116-116: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reject categorical nodes when max_num_categories_ is unset.

If this builder is default-constructed, add_categorical_node() reaches Line 116 with max_num_categories_ == 0, skips external storage even when max_cat_plus_one > bin_width, and get_decision_forest() later reports has_categorical = false. That leaves the forest in an inconsistent state and can under-allocate the categorical bitset backing.

Suggested minimal fix

+    if (max_num_categories_ == index_type{}) {
+      throw model_import_error{
+        "Categorical nodes require max_num_categories to be configured"};
+    }
     if (max_num_categories_ > bin_width) {
       node_value         = categorical_storage_.size();
       auto bins_required = raft_proto::ceildiv(max_cat_plus_one, bin_width);
       categorical_storage_.push_back(max_cat_plus_one);

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@cpp/include/nvforest/detail/decision_forest_builder.hpp` at line 116, The
builder currently allows add_categorical_node() to treat max_num_categories_ ==
0 as if it were large enough, causing has_categorical to become inconsistent and
under-allocating the categorical bitset; update the conditional in
add_categorical_node() (the check using max_num_categories_ and bin_width) to
explicitly reject the case where max_num_categories_ is unset by requiring
max_num_categories_ != 0 (e.g. change the condition to require
max_num_categories_ != 0 && max_num_categories_ > bin_width) and ensure any code
paths that rely on max_cat_plus_one, get_decision_forest(), or the categorical
bitset backing respect this rejection so categorical nodes are only accepted
when a positive max_num_categories_ is configured.